Repository navigation
fix(cli): include external srcset images in project archives - #6335
miguel-heygen merged 2 commits into
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for picking this up as the follow-up from #5309. The change is small and well placed.
What works well:
- Moving the bundler's existing srcset walker into
packages/core/src/compiler/srcset.tskeeps one spec-correct parser instead of adding a second. Its behaviour is unchanged. I checked it against trailing commas,data:candidates, commas inside parenthesised descriptors anda.png,b.png 2x(one URL, per the spec), and all come out right. rewriteHtmlSrcsets(publishProject.ts:545) uses the same resolver, dedup map and relative-path logic assrc/href/poster. A file referenced by bothsrcandsrcsetis archived once. Because targets go throughencodeUrlPath, a name containing a comma or a space cannot break the srcset syntax.- The tests cover the cases that matter: nested entries,
<picture><source>, encoded names, the on-disk staging path, and a control showing that untouched HTML keeps its original bytes.
CI: the required Test check is red only because the Test (runtime) shard hit its 10-minute job timeout while the CLI suite was still running. Every test that ran passed, including the six new ones and the existing publishProject suite. Recent runs finish that shard in about 5 minutes, so this looks like a slow runner. A re-run should clear it.
Optional follow-up, not for this PR: packages/cli/src/cloud/offload.ts:894 still splits srcset on , and skips any srcset that contains data:. Now that the spec walker is exported, cloud-render offload could use it too.
Optional nit: CSS_URL_RE and isPathInside are also exported from the ./compiler subpath (compiler/index.ts:114). If you want rewriteSrcsetUrls there too for consistency, it is a one-line addition. Nothing needs it today.
Happy to approve once the runtime shard is green.
Verdict: COMMENT
Reasoning: The code is correct and reuses the existing parser, but the required Test check is red from a runtime-shard timeout that needs a re-run before approval.
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
The runtime shard that timed out has re-run and every check is now green at e5b1017e, so I'm approving. The change is as I reviewed it at c3bd3ced; the only newer commit merges main.
The two optional follow-ups from my last review (cloud-render offload still splitting srcset on commas, and exporting rewriteSrcsetUrls from the ./compiler subpath) can wait for a later PR.
Verdict: APPROVE
Reasoning: The code is correct and reuses the existing srcset parser, and all checks pass at this head.
|
Approved and queued. One follow-up: external assets (src, href, srcset, url()) can resolve to any file outside the project, which then gets uploaded by publish. Could you send a small follow-up that limits those to media file types? A project referencing something like ../../.aws/credentials shouldn't end up in the archive. |
Publishing a project with
<img srcset="../shared/retina.png 2x">leaves out the image and retains its outside-project path. Include localsrcsetcandidates in the archive and rewrite their URLs, including nested HTML and<picture><source>elements.Reuse the bundler's srcset walker with the existing publishing resolver, deduplication and disk staging. Preserve descriptors, inline data and remote URLs, and retain current percent-encoded filename handling. Source files stay unchanged; HTML with no localized candidate keeps its original bytes.
This is the separate follow-up requested in the review of #5309.
Validation:
#and%. Five fail on unchanged main42b84632; the unchanged-HTML control already passes.